Skip to content

Fm/task/unst 9257 snap longculvert 2#497

Open
FlorisBuwaldaDeltares wants to merge 45 commits intomainfrom
fm/task/UNST-9257_snap_longculvert_2
Open

Fm/task/unst 9257 snap longculvert 2#497
FlorisBuwaldaDeltares wants to merge 45 commits intomainfrom
fm/task/UNST-9257_snap_longculvert_2

Conversation

@FlorisBuwaldaDeltares
Copy link
Contributor

@FlorisBuwaldaDeltares FlorisBuwaldaDeltares commented Jan 15, 2026

What was done

  1. refactor make1d2dlongculverts
  2. snap long culvert endpoints to 2D flownodes
  3. remove long culvert angle correction
  4. write 2d2d contact ids
  5. properly partition contact id's
  6. detect long culvert contact ids during net writing
  7. 2D2D culverts use contact id instead of branch id
  8. modify find1d2dlongculvertlinks algorithm to account for contact ids and snapped flowlinks
  9. crossdefs are always read if they are available in case of no network present
  10. remove most newculvert checks as we now always use the first long culvert flowlink as representative link
  11. more robust direction determination: simply check whether the first node connected is the upstream long culvert flownode

Evidence of the work done

  • Video/figures
    <add video/figures if applicable>
  • Clear from the issue description
  • Not applicable

Tests

  • Tests updated
    <add testcase numbers if applicable, Issue number>
  • Not applicable

Documentation

  • Documentation updated
    <add description of changes if applicable, Issue number>
  • Not applicable

Issue link

@FlorisBuwaldaDeltares FlorisBuwaldaDeltares marked this pull request as ready for review January 26, 2026 17:09

! prevFont = SelectObject (hdc, font)
! Disable invalid operation exceptions temporarily
call ieee_set_halting_mode(ieee_invalid, .false.)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wat is dit nou weer? Interactor probleempje?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ja, interacter OpenGL doet het niet in debug door een openGL floating point exception. Al een langlopend irritatiepuntje van iedereen, en copilot wist toevallig hoe je dit fixte. Als je wil kan het ook in een aparte MR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nee hoor, prima om het erbij te doen

call PetscFinalize(ierr)
if (jampi > 0) then
call mpi_comm_free(PETSC_COMM_WORLD, ierr)
call PetscInitialized(isInitialized, ierr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Het kan maar beter initialized zijn voordat je het finalized. Maar ging dit mis?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ik kreeg een error, maar kan ook zijn dat dat door m'n heap corruption was (die had ik per ongeluk veroorzaakt ergens). Ik revert dit wel

!> simple routine which checks if a given flowlink L is part of a 2D-2D longculvert. Lives here to avoid cyclic dependency.
elemental subroutine is_2D2D_longculvertlink(L, res, i)
integer, intent(in) :: L !< Flowlink number
logical, intent(out) :: res !< Result: true if L is part of a 2D-2D longculvert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je kan ook i = 0 doen als de link niet kan vinden in plaats van res = .false.. Scheelt weer een intent(out) maar misschien vind je dat niet mooi :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oja, het is denk ik niet onduidelijker en scheelt weer een argument, goed idee!

end do
end if

if (allocated(contactnetlinks) .and. allocated(contactids_2D2D)) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly that the 2D2D link is valid only if both endpoints lie in the same partition?

end do

if (newculverts) then
do ilongc = 1, nlongculverts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Floris explained to me what this code was for and why it can be removed. If I understand correctly:
Before the snapping, we would add extra links from the endpoints to the centers, which has a high risk of creating small links at very sharp angles. And this code somehow corrects the flow calculation.

use m_GlobalParameters, only: flow1d_eps10
use precision, only: comparereal
use gridoperations, only: incells
! subroutine longculvert_check_polyline(j, yplCulv, xplCulv)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be removed :)


!> Find 2D netcell the longculvert endpoint is located in, add a new node and return its node number
subroutine longculvert_create_endpoint(j, k)
subroutine longculvert_create_endpoint(x, y, z, k)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better

@@ -1,3 +1,56 @@
# Function to copy all required runtime dependencies for dflowfm_kernel tests
function(copy_dflowfm_test_dependencies TARGET_NAME)
if(WIN32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, no. Maybe move the if(WIN32) outside this function to make it clear that this ugly fix is only needed on windows

<file name="DFM_OUTPUT_tunnel/tunnel_0000_his.nc" type="NETCDF">
<parameters>
<parameter name="longculvert_discharge" toleranceAbsolute="0.005" />
<parameter name="longculvert_discharge" toleranceAbsolute="0.003" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, more checks

</checks>
</testCase>
<testCase name="e02_f014_c150" ref="dimr">
<path version="2025-10-15T15:07:20.418000">e02_dflowfm/f014_parallel/c150_longculvert_parallel</path>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the references for these testcases had to be updated. Too bad, but probably necessary, right?

Copy link
Contributor

@rene-deltares rene-deltares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all my suggestions are optional. Let me know if you need another approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants